Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @useSystemTextJsonConverter decorator for csharp #1230

Closed
wants to merge 29 commits into from

Conversation

live1206
Copy link
Member

@microsoft-github-policy-service microsoft-github-policy-service bot added the lib:tcgc Issues for @azure-tools/typespec-client-generator-core library label Jul 24, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Jul 24, 2024

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - feature ✏️

Add @hasJsonConverter for csharp only to indicate if JSON converter is needed

@live1206 live1206 changed the title Json converter Add @hasJSONConverter decorator Jul 24, 2024
@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@live1206 live1206 changed the title Add @hasJSONConverter decorator Add @hasJSONConverter decorator for csharp Jul 30, 2024
@live1206
Copy link
Member Author

Given the timeline: Azure/autorest.csharp#3469 (comment), we would like to get this out in the next release.

@live1206 live1206 changed the title Add @hasJSONConverter decorator for csharp Add @hasJsonConverter decorator for csharp Aug 1, 2024
- "@azure-tools/typespec-client-generator-core"
---

Add `@hasJsonConverter` for csharp only to indicate if JSON converter is needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets rename to @useSystemTextJsonConverter to make it more explicit what this is used for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll say it feels quite weird having this at the root of Tcgc namespace with this name that is very csharp specific

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is indeed csharp specific only decorator.
@timotheeguerin Are you implying we should keep it within csharp sub-namespace if we ensure it will not be used for other languages?

Copy link
Member

@timotheeguerin timotheeguerin Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just feels like this is not super compatible with the new approach of using a shared name and scope.

But if you feel like it's best then I'm not blocking or anything

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have decided that language-specific decorators will go in the main namespace because of grow-up, but they will have to specify the scope, and we have an issue to have linter warnings if a decorator is applied to emitters that won't do anything with it.

My question with this PR is more: I thought we had decided that there was a use case for Java here too, and that there was going to be a redesign of this decorator (most likely just a rename), that would make it compatible with Java's scenario as well

@live1206 live1206 changed the title Add @hasJsonConverter decorator for csharp Add @useSystemTextJsonConverter decorator for csharp Aug 9, 2024
@live1206
Copy link
Member Author

live1206 commented Aug 9, 2024

merge to august release branch instead in #1336

@live1206 live1206 closed this Aug 9, 2024
tadelesh pushed a commit that referenced this pull request Aug 9, 2024
Resolves #1229
This change has already been approved in
#1230
@live1206 live1206 deleted the json-converter branch August 9, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants